Skip to content

Support using system llhttp library #10760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 22, 2025
Merged

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Apr 20, 2025

What do these changes do?

Add a new USE_SYSTEM_DEPS environment variable that can be used to enable building against the system install of llhttp library rather than the vendored one.

This is implemented using the pkgconfig package to query llhttp install via pkg-config. The package is only used when USE_SYSTEM_DEPS is enabled, however due to limitations of pyproject.toml it is listed unconditionally as a build dependency. That said, flake8 claims that the dependency is not listed and I don't really know how to solve that:

setup.py:36:5: I900 'pkgconfig' not listed as a requirement

I have also updated aiohttp/_cparser.pxd to reference the llhttp.h header via its name rather than full path. I have poisoned the system header to confirm that the correct header is used both when building against the system library and the vendored library.

Are there changes in behavior for the user?

  • no changes for users installing from wheels
  • users installing from source will see an additional pkgconfig build requirement
  • users installing from source who enable USE_SYSTEM_DEPS will be using the system llhttp library

Is it a substantial burden for the maintainers to support this?

I don't think so. The code uses pkg-config, so it should be pretty forward-secure to potentially nonstandard llhttp installs. On top of that, presumably the feature will be primarily used by downstreams / expert users, who would rather be willing to submit fixes, should any issues arise (though that implies the burden of reviewing them).

Related issue number

Discussion #10759.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES/ folder
    • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

    • if you don't have an issue number, change it to the pull request
      number after creating the PR

      • .bugfix: A bug fix for something the maintainers deemed an
        improper undesired behavior that got corrected to match
        pre-agreed expectations.
      • .feature: A new behavior, public APIs. That sort of stuff.
      • .deprecation: A declaration of future API removals and breaking
        changes in behavior.
      • .breaking: When something public is removed in a breaking way.
        Could be deprecated in an earlier release.
      • .doc: Notable updates to the documentation structure or build
        process.
      • .packaging: Notes for downstreams about unobvious side effects
        and tooling. Changes in the test invocation considerations and
        runtime assumptions.
      • .contrib: Stuff that affects the contributor experience. e.g.
        Running tests, building the docs, setting up the development
        environment.
      • .misc: Changes that are hard to assign to any of the above
        categories.
    • Make sure to use full sentences with correct case and punctuation,
      for example:

      Fixed issue with non-ascii contents in doctest text files
      -- by :user:`contributor-gh-handle`.

      Use the past tense or the present tense a non-imperative mood,
      referring to what's changed compared to the last released version
      of this project.

Add a new `USE_SYSTEM_DEPS` environment variable that can be used
to enable building against the system install of llhttp library rather
than the vendored one.

This is implemented using the `pkgconfig` package to query llhttp
install via pkg-config.  The package is only used when `USE_SYSTEM_DEPS`
is enabled, however due to limitations of `pyproject.toml` it is listed
unconditionally as a build dependency.  That said, flake8 claims that
the dependency is not listed and I don't really know how to solve that:

```
setup.py:36:5: I900 'pkgconfig' not listed as a requirement
```

I have also updated `aiohttp/_cparser.pxd` to reference the `llhttp.h`
header via its name rather than full path.  I have poisoned the system
header to confirm that the correct header is used both when building
against the system library and the vendored library.
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 20, 2025
Copy link

codspeed-hq bot commented Apr 20, 2025

CodSpeed Performance Report

Merging #10760 will not alter performance

Comparing mgorny:system-llhttp (7ce83ea) with master (f626aeb)

Summary

✅ 57 untouched benchmarks

Copy link

codecov bot commented Apr 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (b97d6bf) to head (c9d281c).
Report is 20 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10760   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files         125      125           
  Lines       37793    37858   +65     
  Branches     2085     2091    +6     
=======================================
+ Hits        37310    37375   +65     
  Misses        335      335           
  Partials      148      148           
Flag Coverage Δ
CI-GHA 98.60% <ø> (+<0.01%) ⬆️
OS-Linux 98.27% <ø> (+<0.01%) ⬆️
OS-Windows 96.20% <ø> (-0.04%) ⬇️
OS-macOS 97.39% <ø> (+<0.01%) ⬆️
Py-3.10.11 97.31% <ø> (+<0.01%) ⬆️
Py-3.10.17 97.84% <ø> (-0.01%) ⬇️
Py-3.11.12 97.91% <ø> (-0.01%) ⬇️
Py-3.11.9 97.39% <ø> (+<0.01%) ⬆️
Py-3.12.10 98.03% <ø> (-0.01%) ⬇️
Py-3.12.9 97.51% <ø> (-0.01%) ⬇️
Py-3.13.2 97.49% <ø> (+<0.01%) ⬆️
Py-3.13.3 98.03% <ø> (-0.01%) ⬇️
Py-3.9.13 97.18% <ø> (+<0.01%) ⬆️
Py-3.9.22 97.70% <ø> (-0.01%) ⬇️
Py-pypy7.3.16 85.73% <ø> (-8.15%) ⬇️
VM-macos 97.39% <ø> (+<0.01%) ⬆️
VM-ubuntu 98.27% <ø> (+<0.01%) ⬆️
VM-windows 96.20% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'd like this wired via PEP 517 config settings but that's something to implement after redoing packaging similar to yarl. So it's out of the scope.

As for isort, there might be configs you'd need to add the new lib in.

mgorny and others added 3 commits April 22, 2025 03:26
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
@mgorny
Copy link
Contributor Author

mgorny commented May 21, 2025

Gentle ping. Do you have any further requests?

@webknjaz
Copy link
Member

@mgorny hey, sorry, Nick was running the PyCon US sprint almost by himself and I was being pulled into numerous discussions and brainstorming sessions, not being able to catch up with the aiohttp sprint.. I'll try to take a look shortly.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is good bit would like more 👀 just in case..

@webknjaz webknjaz requested review from Dreamsorcerer and bdraco May 22, 2025 03:50
@Dreamsorcerer
Copy link
Member

Not my area of expertise, so I was just waiting on your review. No obvious issues that I see.

@bdraco bdraco added this to the 3.12 milestone May 22, 2025
@bdraco bdraco added the backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot label May 22, 2025
@bdraco bdraco changed the title Support using system llhttp library (#10759) Support using system llhttp library May 22, 2025
@bdraco bdraco merged commit 4152a08 into aio-libs:master May 22, 2025
40 of 41 checks passed
Copy link
Contributor

patchback bot commented May 22, 2025

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/4152a0835b8667827f9740c37c28b83e1d9e9f67/pr-10760

Backported as #10929

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request May 22, 2025
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
(cherry picked from commit 4152a08)
@bdraco
Copy link
Member

bdraco commented May 22, 2025

Thanks @mgorny

bdraco pushed a commit that referenced this pull request May 22, 2025
…ry (#10929)

Co-authored-by: Michał Górny <[email protected]>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
@mgorny mgorny deleted the system-llhttp branch May 22, 2025 15:37
@mgorny
Copy link
Contributor Author

mgorny commented May 22, 2025

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants